replace journal swift extension components with lui elements - #31
Conversation
- bump lui pin to 69b3ffd (edge-chrome, media, controls, file-picker, list suite) - Native_list: lui list/list_section/list_item/swipe_action + scroll/visible-range events - journal_header: edge_inset/overlay/view_that_fits chrome - journal_media_view: file_image/button/link + QuickLook file_preview + dropdown actions - journal_asset_import: file_picker request/staged-completion protocol - journal_asset_settings: navigation-form sheet + number_stepper (days via new LJP2 tags 28-31) - delete JournalChrome/JournalList/JournalMedia/JournalAssetImport/JournalAssetSettings/JournalExtensions + journal_lui_native - App/RuntimeHost/Runtime drop extension registry; outline probe + adaptive tests use lui list/create
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
- row-level on_press/trailing icon on list_item instead of nested Navigation_link list_item (rejected by list-item child rules) - attachment sheet drops unsupported min-width/min-height props - toolbar gets required accessibility ~label - journals header controls row packs trailing via main=end_ (lui rows auto-append a trailing spacer on default main) - pending-chip remove button gets ~label (icon-only buttons require an accessible name) - media root/item emit root/asset visibility on_appear so the media runtime tracks browsed roots - bump lui pin: file-picker anchors presentation modifiers on a 1pt placeholder when childless
|
iOS-sim verification (journal 52b5a4f, lui 284730e, iPhone 17 / iOS 26.5, E2EE warm-mirror graph) Passes: timeline + chrome (account capsule top-right, bottom capsule bar) · row press → Block detail nav · detail outline renders immediately (disclosure expand/collapse, per-row context menus) · Append sheet/send · Attachment settings sheet at medium detent + One lui host regression found + fixed in this branch's pin: the detail page's
Pre-existing/environmental issue (not a regression): media items enumerate correctly (new Not exercised: |
lui PRs #70-74 (edge chrome, file image/preview/link, number stepper + sheet sizing, file picker, list suite) plus the edge-inset pinned-region fix are now all on main; repoint the pin from the integration branch to the merge SHA.
|
Re-verified on merged lui main (journal 15bb54a, lui 12d95ad) Smoke pass on iPhone 17 sim — all green: timeline renders, Block detail opens instantly with full outline (edge-inset fix confirmed on merged main), disclosure expand/collapse works, composer doc-picker presents and stages a file chip. No schema violations or crashes in stderr. Known outstanding (unchanged): media items stall at "Waiting for file" — pre-existing/environmental in the service fetch pipeline per prior diagnosis. |
Keep the lui file-picker decode path; carry over the base's request-id echo check by extracting the id from the armed journal-import token, and keep discard_staged_file.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
f06948a
into
devin/1790580543-composer-assets
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Devin resolved all 5 findings on e8751e7
Fixed by Devin (5)
- Extensionless files cannot be attached
- Large attachment picks exhaust app memory
- Later attachment picks receive premature completion
- Old graph preview persists after switching
- TIFF and AVIF previews disappear
View all findings in Devin Review
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8751e71c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let appear = emit_handler ~root ~on_event "root" () in | ||
| let content = | ||
| Lui_elements.column | ||
| ~gap:8 | ||
| ~cross:`start | ||
| ~on_appear:(fun _ -> Ui.Event.Handler.Private.invoke appear Ui.Event.Payload.Unit) |
There was a problem hiding this comment.
When a user scrolls through more than 64 blocks without changing routes, this only emits root visibility on appearance and every payload hard-codes visible = true; there is no corresponding disappearance event. Journal_media_runtime.root_visible can evict only groups whose visible flag is false, so the first 64 visited blocks remain resident and the 65th block is never registered, preventing its attachments from loading while also retaining obsolete foreground demands. Wire the native disappearance lifecycle to emit visible = false for roots and assets.
Useful? React with 👍 / 👎.
| let ic = open_in_bin path in | ||
| let length = in_channel_length ic in | ||
| let contents = really_input_string ic length in | ||
| close_in ic; | ||
| let oc = open_out_bin dest in | ||
| output_string oc contents; |
There was a problem hiding this comment.
When the Files picker selects a large attachment, really_input_string reads the entire file into the OCaml heap synchronously on the UI event path before writing it out. Since the picker accepts unrestricted file types, a large video, archive, or document can freeze the app or terminate it for memory pressure; copy the file with a bounded streaming loop or a native filesystem copy instead.
Useful? React with 👍 / 👎.
| journal_ids | ||
| journal_lui_native | ||
| journal_pump |
There was a problem hiding this comment.
This commit changes app/dune by removing journal_lui_native, but the repository instructions explicitly prohibit modifying any dune file unless that modification was requested. Restore the module/build arrangement or obtain explicit authorization and rescope the change accordingly.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| @@ -1,181 +0,0 @@ | |||
| import LUIAppleBackend | |||
There was a problem hiding this comment.
Deleting this file leaves tool/test_swiftui_warm_start.py:80 referencing JournalChrome.swift; the script hashes every listed source with read_bytes() at line 83, so the documented warm-start acceptance command now always raises FileNotFoundError before staging or building the probe. A repository-wide search found this as the remaining non-documentation reference, so update that source list as part of the deletion.
Useful? React with 👍 / 👎.
| let staged_completion request pending = | ||
| match pending with | ||
| | [] -> None | ||
| | _ :: _ -> Some (request_token request, None) |
There was a problem hiding this comment.
After one attachment is pending, arming a second picker increments request.id, but this function immediately derives the completion token from that new request merely because the old pending list is nonempty. The picker therefore receives request 2 and completion 2 before request 2 has produced a selection; this can prematurely complete that picker or prevent its eventual retained selection from observing a completion transition. Preserve the originating request token with the picked item and acknowledge only that token so captures can reliably add multiple attachments.
Useful? React with 👍 / 👎.
| ; (if row.retry | ||
| then | ||
| Lui_elements.button | ||
| ~text:"Retry" | ||
| ~accessibility_identifier:("journal-upload-retry:" ^ row.id) | ||
| ~on_press:(fun _ -> on_event ("retry:" ^ row.id)) |
There was a problem hiding this comment.
When multiple uploads require attention, every retry control is now exposed only as “Retry,” whereas the removed Swift implementation labeled each button “Retry upload: <title>.” VoiceOver users cannot determine which failed upload a button will retry without that item-specific accessible name, so pass a label derived from row.title while retaining the visible short text.
Useful? React with 👍 / 👎.
| ] | ||
| @ List.map (upload_row ~on_event) uploads | ||
| @ [ secondary recent; secondary favorites ]) |
There was a problem hiding this comment.
When the attachment settings sheet is open, these two status strings are rendered consecutively without indicating which describes recent journals and which describes favorites. Common states produce identical text such as “Waiting for a graph” or “All attachments available offline,” making the values impossible to distinguish; the removed native view prefixed them with “Recent journals:” and “Favorites:”, which should be preserved.
Useful? React with 👍 / 👎.
| let controls = | ||
| Lui_elements.row | ||
| ~main:`end_ | ||
| ~gap:8 | ||
| ((if connecting then [ Ui.mount (V.progress ~style:Circular ()) ] else []) |
There was a problem hiding this comment.
When the Journals screen is in Graph_service.Connecting, this raw circular progress node is the only connection feedback in that context, but it has no accessible label. The removed chrome implementation explicitly exposed the spinner as “Connecting,” so VoiceOver users now receive an unnamed progress indicator; attach the same semantic label or use the labeled loading component.
Useful? React with 👍 / 👎.
1 participant
Summary
Replace all five journal-specific Swift extension components in
swift/with the generic elements now shipped inlogseq/lui@12d95ad(main, PRs #70-#74 merged). After this PR the journal mounts only standard LUI elements; the extension registry (journal_lui_native.ml) and theJournal*.swiftextension files are deleted.Element mapping:
JournalChrome(safeAreaInset + corner overlay + ViewThatFits)edge-inset,overlay,view-that-fits(lui#70)JournalMedia(path image, QuickLook preview, external link)file-image,file-preview,link(lui#71)JournalAssetSettings(numeric stepper, sheet detents)number-stepper, sheetdetents/sizingprops (lui#72)JournalAssetImport(file/photo/camera pickers)file-pickersource∈ files/photos/camera (lui#73)JournalList(grouped/disclosure/swipe/scroll-target/visible-range)list,list-section,list-itemsuite (lui#74)OCaml call sites updated accordingly:
Journal_view.Native_listnow buildsLui_elements.list/list_section/list_itemtrees (disclosure children nest aslist_itemchildren;context-menumounts on the item;scroll_target_keycollapses to the leaf key), andjournal_media_view/journal_asset_import_view/journal_asset_settings_view/journal_headermount the new generic elements.Note:
app/duneloses thejournal_lui_nativemodule entry — required for removing the extension registry.Regression found and fixed during verification:
LUIEdgeInsetView's pinned region in.safeAreaInsetwas unbounded, so an expansive pinned child (the detail overlay) consumed the whole inset and collapsed the base list to zero height (blank detail page). Fix inLUISwiftUIRoot.swift—.fixedSizeon the pinned stacks, merged to main via lui#70. Verified on iPhone 17 sim: detail renders, disclosure rows and context menus work, hit-testing unaffected.Known pre-existing issue (not a regression, reproduced on
mainsemantics): media items enumerate but downloads never reach transport — "0 of 5 attachments available offline". Service path (journal_media*,logseq_sync,logseq_db_worker) is byte-identical to main; tracked separately.Link to Devin session: https://app.devin.ai/sessions/55c3d31f1c6d4ee4beda2f832725d7b7
Open in Devin Desktop: https://app.devin.ai/desktop/session/55c3d31f1c6d4ee4beda2f832725d7b7?variant=devin
Requested by: @RCmerci